-
Notifications
You must be signed in to change notification settings - Fork 5k
chore: metricbeat: openai: Implement ReportingMetricSetV2WithContext #47387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Switches the openai module to ReportingMetricSetV2WithContext to get rid of a context.TODO().
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
stefans-elastic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@elastic/obs-infraobs-integrations can I get a review here please? |
|
@lalit-satapathy could we please have someone to take a look here? |
|
We are actually not using this module for OpenAI Observability in Integrations. The CEL input is being used. |
|
Hi @orestisfl! We do not support this OpenAI module; in fact, we never actually utilized it. Our initial plan was to use this module for integration, but immediately after we merged it, OpenAI announced their official Usage API. Consequently, we developed the solution in CEL instead. The CEL approach allows us to iterate faster and makes updates easier to deploy. This module relies on an undocumented API from OpenAI. While that was originally the only way to collect data, it is now obsolete. Since we have the official API, we neither use nor support this module. Furthermore, because OpenAI no longer maintains the undocumented endpoint, users should avoid this module entirely. Therefore, we are no longer accepting changes to this module. I will add a deprecated comment, I will see how to do it and will do the needful. We will only make an exception for changes that are critical blockers for other components. We will not accept any non-blocking changes as this module is no longer maintained. In case you want to merge as this could be a blocker — like removing some function or use of some package or something of that nature that could be a blocker, please let us know. I will approve those PRs. |
Proposed commit message
Switches the openai module to ReportingMetricSetV2WithContext to get rid of a
context.TODO().Taking a look in persistcache.go, I see that we probably don't really need any of the two
sync.RWMutexes since the only write-able state is in async.Map.Checklist
I have commented my code, particularly in hard-to-understand areasI have made corresponding changes to the documentationI have made corresponding change to the default configuration filesstresstest.shscript to run them under stress conditions and race detector to verify their stability.I have added an entry in./changelog/fragmentsusing the changelog tool.How to test this PR locally
Related issues